Update BrowserSignin initializer to not throw unnecessarily - #251
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the BrowserSignin initializer to remove unnecessary throwing behavior by directly using non-throwing AuthorizationCodeFlow and SessionLogoutFlow initializers instead of relying on OAuth2Client's throwing initializer.
- Replaces throwing BrowserSignin convenience initializers with non-throwing versions
- Updates AuthorizationCodeFlow convenience initializers to accept an optional logoutRedirectUri parameter
- Modifies test code to use the new non-throwing initialization patterns
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Sources/BrowserSignin/BrowserSignin.swift | Removes throws from convenience initializers and restructures initialization to use AuthorizationCodeFlow and SessionLogoutFlow directly |
| Sources/OAuth2Auth/Authentication/AuthorizationCodeFlow.swift | Adds logoutRedirectUri parameter to convenience initializers |
| Tests/BrowserSigninTests/BrowserSigninInitializerTests.swift | Updates test to use non-throwing initializer |
| Tests/BrowserSigninTests/BrowserSigninFlowTests.swift | Refactors tests to manually create flows instead of using BrowserSignin convenience initializer |
| Sources/AuthFoundation/JWT/Protocols/Claim.swift | Adds Sendable, Hashable, Equatable conformance to IsClaim protocol |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
AlexNachbaur
requested review from
IldarAbdullin-okta and
sandeeppenchala-okta
September 11, 2025 17:48
AlexNachbaur
assigned
AlexNachbaur and unassigned
IldarAbdullin-okta and
sandeeppenchala-okta
jaredperreault-okta
approved these changes
Sep 11, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
BrowserSignininitializer was only throwing because theAuthorizationCodeFlow's initializer being used was throwing if theredirectUriwas nil. However, BrowserSignin was accepting a non-optional URL for that same value. As a result, AuthorizationCodeFlow would never throw.Instead of using
try!to prevent this (since the initializer may change in the future), I rearranged the code to utilize initializers that would prevent needing to use throws.